Skip to content
This repository has been archived by the owner on Apr 10, 2021. It is now read-only.

Add initial version of a coding guide #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pcahyna
Copy link
Contributor

@pcahyna pcahyna commented Apr 22, 2020

For more in-depth explanations than in the coding standards.

File name is up to discussion.

@richm
Copy link
Contributor

richm commented Apr 22, 2020

lgtm

Copy link
Contributor

@richm richm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

GUIDE.md Outdated
expanded only when `__timesync_chrony_version` is used, and if it is
after we collect the fact, everything is fine.

(Beware though, `set_facts` expands variables, because it is is like a
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^is is^is^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @nhosoi. Corrected.

For more in-depth explanations than in the coding standards.
@richm
Copy link
Contributor

richm commented May 22, 2020

Can this be merged?

properly.

Another place where this is used is here:
https://github.com/linux-system-roles/timesync/blob/9050dd95314406236bc8869eac2307a6fa932edc/vars/main.yml#L1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a link for readability.

to the task, so the variable would be unset both before and after the
task.
See
https://github.com/ansible/ansible/issues/66714
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these into links for readability.


[2] Another well-known language with this style of variable expansion
is `make`:
https://www.gnu.org/software/make/manual/html_node/Flavors.html
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a link for readability.

something like `role_name` to another role and use it to reference to
the calling role, because the variable will only get expanded in the
called role to something else - the variable represents the currently
executing role. https://github.com/ansible/ansible/issues/10374
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a link for readability.

Variables set on the `roles` or `import_role` statement appear like
parameters, but actually they are made accessible to the whole
playbook run, just like role variables set in `vars/` - therefore can
influence further role invocations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's worth notiing that this can be made to happen with include_role too: https://docs.ansible.com/ansible/latest/modules/include_role_module.html#parameter-public

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try that - does it really have the same effect on the variables given in the vars keyword to include_role as in the case of import_role? The effect can't be exactly the same, because variables given on import_role are visible even before this task, while public makes the variables visible only after the include_role task.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point.

influence further role invocations.

Example:
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we toss "```yaml" here for syntax highlighting?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants